-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Simulate ingest API uses existing index mapping when mapping_addition is given #132101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simulate ingest API uses existing index mapping when mapping_addition is given #132101
Conversation
Hi @masseyke, I've created a changelog YAML for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug in the simulate ingest API where when an index exists and a mapping_addition
is provided, the API incorrectly uses the mapping from the index's matching template instead of the actual index mapping. The fix ensures that the existing index mapping is always used as the base when merging with mapping_addition
.
Key changes:
- Simplified the condition check to always use existing index mapping when no template substitutions are present
- Removed duplicate code path that incorrectly handled mapping addition for existing indices
- Added comprehensive test coverage for both regular indices and data streams
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
TransportSimulateBulkAction.java | Main fix - simplified logic to always use existing index mapping when available, removed duplicate handling code |
80_ingest_simulate.yml | Added extensive test cases covering the bug scenarios for both regular indices and data streams |
132101.yaml | Added changelog entry documenting the bug fix |
server/src/main/java/org/elasticsearch/action/bulk/TransportSimulateBulkAction.java
Show resolved
Hide resolved
Pinging @elastic/es-data-management (Team:Data Management) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the comments in the yaml tests, they're nice to have to follow the test.
CompressedXContent mappings = imd.mapping() == null ? null : imd.mapping().source(); | ||
CompressedXContent mergedMappings = mappingAddition == null ? null : mergeMappings(mappings, mappingAddition); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These … == null ? null : …
are calling out for Optional.map()
:D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha yeah I thought about doing that but then didn't (for reasons I don't remember). I'll do that now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, because mergeMappings throws a checked exception, and dealing with that checked exception makes the code far more complicated than just the null checks. I'll change the first line though.
…seyke/elasticsearch into fix/simulate-ingest-mapping-addition
💔 Backport failed
You can use sqren/backport to manually backport by running |
💔 Some backports could not be created
Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
… is given (elastic#132101) (cherry picked from commit 593f48f) # Conflicts: # server/src/main/java/org/elasticsearch/action/bulk/TransportSimulateBulkAction.java
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
… is given (elastic#132101) (cherry picked from commit 593f48f) # Conflicts: # server/src/main/java/org/elasticsearch/action/bulk/TransportSimulateBulkAction.java
This fixes a bug described at #131608 (comment). The problem is that if an index exists and a
mapping_addition
is given, we ignore the mapping from the index and instead use the mapping from the index's matching template if it exists (otherwise the index's mapping is used).